fix(security): close cross-tenant IDOR gaps in OAuth credential and execution auth#4549
Conversation
…orization Routes that called resolveOAuthAccountId followed by a conditional workspace permission check (only run when workspaceId was set) silently skipped all ownership validation on the legacy account-ID fallback path. Any authenticated user could supply a raw account.id to access another tenant's OAuth credentials. - Replace resolveOAuthAccountId + conditional perm check with authorizeCredentialUse in: auth/oauth/wealthbox/item, tools/gmail/label, tools/onedrive/files, tools/onedrive/folder, tools/outlook/folders, tools/wealthbox/item (routes 1, 3-7) - Add authorizeCredentialUse ownership gate before resolveVertexCredential in providers/route.ts (route 2) - Add verifyFileAccess check on the user-supplied file key before downloadFileFromStorage in tools/wordpress/upload (route 8) - Add workflowId param to PauseResumeManager methods (enqueueOrStartResume, beginPausedCancellation, completePausedCancellation, blockQueuedResumesForCancellation, clearPausedCancellationIntent, getPausedCancellationStatus, processQueuedResumes) and filter all pausedExecutions lookups by workflowId so callers cannot act on another tenant's paused execution by supplying a foreign executionId (route 9) - Update all call sites (cancel, resume, poll routes) to pass workflowId
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Hardens file transfer by verifying the caller owns the referenced storage key ( Scopes human-in-the-loop pause/resume/cancellation operations by requiring Reviewed by Cursor Bugbot for commit 026405f. Configure here. |
Greptile SummaryThis PR closes a suite of cross-tenant IDOR vulnerabilities by replacing ad-hoc
Confidence Score: 5/5Safe to merge — all cross-tenant credential and execution-control gaps are consistently closed with no behavioral regressions found. All three fix categories (OAuth credential IDOR, HITL cancel/resume IDOR, WordPress file ownership) are correctly and completely implemented. Previously flagged concerns about processQueuedResumes, getPausedCancellationStatus, and completePausedCancellation missing workflowId are fully addressed. The authorizeCredentialUse refactor removes dead actingUserId guards without changing observable behavior. No files require special attention — the core security logic in credential-access.ts and human-in-the-loop-manager.ts was reviewed thoroughly and is correct. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant ToolRoute as Tool Route (gmail, onedrive, etc.)
participant ACA as authorizeCredentialUse
participant DB as Database
participant OAuthUtils as refreshAccessTokenIfNeeded
Client->>ToolRoute: "GET /api/tools/... ?credentialId=X"
ToolRoute->>ACA: "authorizeCredentialUse(request, {credentialId})"
ACA->>DB: checkSessionOrInternalAuth
ACA->>DB: "SELECT FROM credential WHERE id = credentialId"
alt Platform credential (OAuth/SA)
ACA->>DB: SELECT FROM credential_member WHERE credentialId AND userId
ACA->>DB: getUserEntityPermissions(actingUser, workspace)
ACA-->>ToolRoute: ok: true, credentialOwnerUserId
else Legacy account ID
ACA->>DB: "SELECT FROM account WHERE id = credentialId"
ACA->>ACA: "auth.userId === account.userId?"
ACA-->>ToolRoute: ok: true, credentialOwnerUserId
end
ToolRoute->>OAuthUtils: refreshAccessTokenIfNeeded(credentialId, ownerUserId)
OAuthUtils-->>ToolRoute: accessToken
ToolRoute-->>Client: 200 with data
Reviews (6): Last reviewed commit: "fix(security): harden workflowId scoping..." | Re-trigger Greptile |
…ocessQueuedResumes, fix log level - Fail closed in WordPress upload when userFile.key is present but authResult.userId is absent, preventing silent bypass of ownership check via JWT fallback path - Thread workflowId into processQueuedResumes in the async resume error-recovery path and in pause-persistence.ts to close residual cross-tenant gap - Change logger.error to logger.warn for credential access denial in OneDrive folder route to match all other routes in this PR
|
@cursor review |
|
@greptile |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 94bc2e2. Configure here.
…l sites Closes residual cross-tenant IDOR gap where processQueuedResumes was called without a workflowId scope in persistPauseResult, startResumeExecution (success and error paths), and clearPausedCancellationIntent. workflowId was already in scope at each site — this wires it through to the existing optional parameter.
|
Follow-up fix (fd234ac): threaded |
…caught errors - catch (error: any) → catch (error) + toError(error).message in resume and cancel routes - Remove what-not-why inline comments from wordpress upload and onedrive/files routes - Remove redundant debug-only item breakdown log and the file-IDs log in onedrive/files - Trim extraneous DAG-edge comments from updateSnapshotAfterResume in HITL manager
|
Quality pass complete (0d4c9c6):
All 4 original review threads were already resolved before this pass. Lint is clean. |
|
@cursor review |
|
@greptile |
|
@cursor review |
|
@greptile |
All 7 method signatures (processQueuedResumes, enqueueOrStartResume, beginPausedCancellation, completePausedCancellation, blockQueuedResumesForCancellation, clearPausedCancellationIntent, getPausedCancellationStatus) previously accepted workflowId as optional. Every call site already supplies it — making it required closes the vulnerability at the type level so future callers cannot accidentally omit tenant scoping and silently fall back to an unscoped DB query.
…alls and remove dead branches in credential-access
|
@greptile |
|
@cursor review |
Replace falsy workflowId checks in PauseResumeManager (all methods now unconditionally apply the workflowId WHERE clause, preventing empty-string bypass). Flip WordPress upload file guard from truthy key check to explicit non-empty validation so key:"" fails closed with a 404 instead of silently skipping access control.
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 026405f. Configure here.
Summary
gmail/label,onedrive/files,onedrive/folder,outlook/folders,wealthbox/item,auth/oauth/wealthbox/item) calledresolveOAuthAccountIdthen only checked workspace permissions whenworkspaceIdwas set — the legacy account-ID fallback path returned noworkspaceId, silently skipping all ownership validation. Any authenticated user could supply a rawaccount.idto access another tenant's OAuth credentialsauthorizeCredentialUse, which enforces ownership on every credential type including the legacy fallback pathauthorizeCredentialUseownership gate beforeresolveVertexCredentialinproviders/route.ts(Vertex AI credential path had the same gap)verifyFileAccesscheck inwordpress/uploadbeforedownloadFileFromStorage— file key was user-supplied with no ownership verificationworkflowIdfilter to allPauseResumeManagermethods (enqueueOrStartResume,beginPausedCancellation, etc.) — previously looked up paused executions byexecutionIdonly, allowing any authenticated user to cancel or resume another tenant's paused workflow execution; update all call sites (cancel, resume, poll routes)Type of Change
Testing
Tested manually.
authorizeCredentialUsebackwards-compatibility verified: legacy account-ID callers who own their credentials still work (ownership enforced); HITL managerworkflowIdparam is optional so internal self-calls without it are unaffected;verifyFileAccessconfirmed to exist and match usage.Checklist